Repository navigation
Fix plot_conditions in analysis.utils raising KeyError and drawing empty subplots - #330
Merged
pellet merged 2 commits intoOct 5, 2026
Merged
Conversation
Three problems made this function unusable. Calling it with its documented defaults raised KeyError. `diff_waveform` defaults to the marker codes (1, 2) but was used to index `conditions`, which is keyed by condition label. With `diff_waveform=None` it produced empty subplots. Rows were selected with `dfX.condition.isin(<marker codes>)`, but the condition column made by `epochs.to_data_frame()` holds event names, so nothing ever matched. Amplitudes were scaled twice. `to_data_frame()` already converts EEG from volts to microvolts, and the function multiplied by 1e6 again, putting every trace a millionfold outside the default ylim of (-6, 6). Resolve marker codes to event names through `epochs.event_id`, use that for both the conditions and the difference waveform, and ask `to_data_frame` for microvolts once.
Collaborator
|
Hi Aditya. Thanks for this contribution. Is this PR intended to fix the plot here? https://neurotechx.github.io/EEG-ExPy/auto_examples/visual_n170/01r__n170_viz.html When I run I don't see any difference. Fixing that would be the main priority. |
Contributor
Author
|
No, this one does not affect that plot. #329 is the one for that page. On master |
pellet
added a commit
to pellet/EEG-ExPy
that referenced
this pull request
Sep 1, 2026
The doc build cache key only hashed examples/**/*.py, doc/**/*, and conf.py. A change to library code under eegnb/ (e.g. NeuroTechX#330) still counts as a 'full build' per the earlier changed-files check, but the cache step restores the same doc/_build/html as before since its key is unchanged, and sphinx-gallery skips re-running any example script that itself is unchanged. Net effect: CI can report success without ever re-rendering the affected example. Also there was no way to see a PR's doc build without checking it out and building locally - docs.yml only publishes to GitHub Pages on push to master. Upload the built HTML as a workflow artifact on every run so reviewers can download and open it directly from the PR's checks tab.
With NeuroTechX#330, eegnb.analysis.utils.plot_conditions no longer scales amplitudes twice, so the example's million-scale axis limits flatten the traces. Remove them and use the helper's default microvolt range.
pellet
approved these changes
Oct 5, 2026
pellet
left a comment
pellet
left a comment
Collaborator
There was a problem hiding this comment.
Thanks, this fixes all three bugs, and I checked it against the real N170 data.
- Since 1a5b387 the N170 example imports
plot_conditionsfromeegnb.analysis.utils, so this PR does now change that page. The example's hard-coded limits (±0.5e6and-1.5e6..2.5e6) were sized for the old double scaling, so with your fix the traces would be drawn flat. I've pushed a commit to this branch that removes those three lines. The figure then renders at the default ±6 µV, and the plotted averages match MNE'sepochs.average(). - The new tests pass on this branch and fail on master.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
plot_conditionsineegnb/analysis/utils.pycannot currently produce a correct plot under any argument combination. This is the copy of the function re-exported byeegexpy/__init__.py, andeegnb/analysis/utils.pyis the module the installation docs point new users at, so it is worth having working. Note that this is a different copy from the one inanalysis_utils.pythat the examples import, which I sent separately in #329. The two copies have drifted and are broken in different ways.Calling it the documented way raises.
diff_waveformdefaults to(1, 2), documented as a tuple of marker codes, but the difference branch doesconditions[diff_waveform[1]], looking those codes up as keys of the conditions dict. With the usual label-keyed conditions such asOrderedDict(NonTarget=[1], Target=[2]),plot_conditions(epochs, conditions)fails withKeyError: 2.Passing
diff_waveform=Nonegets past that and then returns a figure with nothing in it. Rows are picked withdfX.condition.isin(conditions[cond_name]), where the right hand side is marker codes like[1], but theconditioncolumn built byepochs.to_data_frame()holds the event names fromevent_id, so the comparison never matches and every subplot is empty.Amplitudes are also scaled twice.
epochs.to_data_frame()already applies MNE's default scaling of 1e6 for EEG, and the function then doesdfX[channel_names] *= 1e6on top. A 3 uV signal therefore reaches the axis as 3000000, whileylimdefaults to(-6, 6), so even once the selection is fixed the traces sit a millionfold off screen.The fix inverts
epochs.event_idto translate marker codes into the event names the data frame is labelled with, routes both the per-condition selection and the difference waveform through that, and requests microvolts fromto_data_frameonce rather than rescaling afterwards. Values that are already names still pass through, so callers who were working around this by passing names are unaffected.I added
tests/test_analysis_utils_plots.py, which builds synthetic MNE epochs with one exact amplitude per condition, so no hardware and no downloaded dataset are needed. Against current master all 3 fail, withKeyError: 2fromutils.py:294for the default-arguments test and the amplitude test, andchannel 0: expected one line per condition, got 0for the empty-figure test. With the change all 3 pass, and the traces come out at 2.00 uV, 5.00 uV and a 3.00 uV difference, matching the synthetic input.mypyis clean on both files.